Skip to content
This repository has been archived by the owner on Sep 13, 2023. It is now read-only.

Refactoring of deployments and env #417

Merged
merged 30 commits into from
Oct 5, 2022
Merged

Refactoring of deployments and env #417

merged 30 commits into from
Oct 5, 2022

Conversation

mike0sv
Copy link
Contributor

@mike0sv mike0sv commented Sep 23, 2022 •
edited
Loading

This is last bits on refactoring deployments. It includes:

  • move deploying logic from envs to deployments
  • remove model field from deployment, now you should provide it every time you run deployment
  • changes to mlem deployment run similar to those in Add -c help WIP #363
  • add used declaration to state to detect changes (eg change in heroku app name). For now we should just forbid changes (ie just create new deployment and remove old one). In future we can allow changing some of the fields (scaling factor, for example)
  • add used model info to state. For now it will be only path and project (if used explicitly). In future we should also add rev for git-tracked models (commit sha, not branch/tag) to link deployment state with repo "state". Also a good idea will be to have an option to forbid deploying not tracked models (ie changed after last commit) or at least warn about it. This will allow users to enforce this linking and have exactly the same version of model in commit and in deployment

Some examples of how deployments cli can be used:

# declare everything separately
mlem declare env heroku --api_key lol prod.mlem
mlem declare deployment heroku --env prod.mlem --app_name myapp service.mlem
mlem deployment run --load service.mlem --model mdoel
# ----
# declare deployment with embedded env
mlem declare deployment heroku --env.api_key lol --app_name myapp service.mlem
mlem deployment run --load service.mlem --model mdoel
# ----
# declare env only
mlem declare env heroku --api_key lol prod.mlem
mlem deployment run heroku service.mlem --model model --app_name myapp --env prod.mlem
# ---
# provide all parameters to `run
mlem deployment run heroku service.mlem --model model --app_name myapp --env.api_key lol
# error on args mismatch

@mike0sv mike0sv requested review from aguschin, omesser, madhur-tandon and a team September 23, 2022 15:11
@mike0sv mike0sv self-assigned this Sep 23, 2022
@mike0sv mike0sv temporarily deployed to internal September 23, 2022 15:11 Inactive
@mike0sv mike0sv temporarily deployed to internal September 23, 2022 15:11 Inactive
@mike0sv mike0sv temporarily deployed to internal September 23, 2022 15:11 Inactive
@madhur-tandon
Copy link
Contributor

  • move deploying logic from envs to deployments

Had a quick look, this is basically reversing the roles of envs and deployments.
But I think tests have to be changed right now? to accommodate this reversal of roles.

  • remove model field from deployment, now you should provide it every time you run deployment

I guess this only applies to mlem deployment run, what about other sub-commands such as mlem deployment apply? They shouldn't be affected right? or --model has to be used with each sub-command as well?

Also, just to confirm, if I understand correctly, I guess the commands will change in such a manner right?:
BEFORE:

mlem deployment run testsk --model sk-model --env kubernetes

AFTER:

mlem deployment run kubernetes testsk.mlem --model sk-model

@mike0sv
Copy link
Contributor Author

mike0sv commented Sep 29, 2022

But I think tests have to be changed right now?

They are.

Every command except run works with already existing deployment so you do not need to provide model option.

Now you can either create new deployment with mlem deployment run <type> <path> [options]
or run existing with mlem deployment run --load <path>

@mike0sv mike0sv temporarily deployed to internal September 29, 2022 13:43 Inactive
@mike0sv mike0sv temporarily deployed to internal September 29, 2022 14:30 Inactive
@mike0sv mike0sv temporarily deployed to internal September 29, 2022 14:55 Inactive
@mike0sv mike0sv temporarily deployed to internal September 29, 2022 19:02 Inactive
@mike0sv mike0sv temporarily deployed to internal September 29, 2022 20:09 Inactive
@mike0sv mike0sv temporarily deployed to internal September 29, 2022 20:14 Inactive
@mike0sv mike0sv temporarily deployed to internal September 30, 2022 14:43 Inactive
@codecov
Copy link

codecov bot commented Sep 30, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (release/0.3.0@0dbe116). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files 
@@               Coverage Diff                @@
##             release/0.3.0     #417   +/-   ##
================================================
  Coverage                 ?   87.62%           
================================================
  Files                    ?       94           
  Lines                    ?     7847           
  Branches                 ?        0           
================================================
  Hits                     ?     6876           
  Misses                   ?      971           
  Partials                 ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mike0sv mike0sv temporarily deployed to internal October 1, 2022 11:50 Inactive
@mike0sv mike0sv temporarily deployed to internal October 3, 2022 14:36 Inactive
madhur-tandon and others added 2 commits October 3, 2022 21:13
@madhur-tandon @mike0sv
fix issues with pandas pylint and flake8 (#427)
779b8b3 
* fix issues with pandas pylint and flake8

* fix requirements

* fix req tests

* remove comment

* fix catboost req tests

Co-authored-by: mike0sv <[email protected]>
@mike0sv
Merge branch 'main' into feature/deployments-cli
3c8be0f
@mike0sv mike0sv temporarily deployed to internal October 3, 2022 17:09 Inactive
@mike0sv mike0sv temporarily deployed to internal October 3, 2022 18:00 Inactive
@mike0sv mike0sv temporarily deployed to internal October 3, 2022 21:20 Inactive
@mike0sv mike0sv temporarily deployed to internal October 3, 2022 23:55 Inactive
@mike0sv mike0sv temporarily deployed to internal October 4, 2022 06:22 Inactive
@mike0sv mike0sv temporarily deployed to internal October 4, 2022 10:20 Inactive
@mike0sv mike0sv temporarily deployed to internal October 4, 2022 13:36 Inactive
@mike0sv mike0sv temporarily deployed to internal October 4, 2022 14:04 Inactive
aguschin
aguschin approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@aguschin aguschin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

mlem/cli/deployment.py Outdated Show resolved Hide resolved
mlem/contrib/docker/base.py Outdated Show resolved Hide resolved
mlem/contrib/docker/base.py Outdated Show resolved Hide resolved
mlem/contrib/heroku/meta.py
@@ -53,11 +62,12 @@ def ensured_app(self) -> HerokuAppMeta:
return self.app


class HerokuDeployment(MlemDeployment):
class HerokuDeployment(MlemDeployment[HerokuState, HerokuEnv]):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confused how to read this type hint. Why it could be MlemDeployment[Y, Z]? Could you please explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for mypy and IDE help mostly. MlemDeployment is Generic and methods like get_env will return not arbitrary MlemEnv, but whatever you provide as second generic parameter (HerokuEnv in this case).

@mike0sv @aguschin
Apply suggestions from code review
0bd86f9 
Co-authored-by: Alexander Guschin <[email protected]>
@mike0sv mike0sv temporarily deployed to internal October 5, 2022 10:29 Inactive
@mike0sv @aguschin
Update mlem/cli/deployment.py
d60f3cf 
Co-authored-by: Alexander Guschin <[email protected]>
@mike0sv mike0sv temporarily deployed to internal October 5, 2022 10:30 Inactive
@mike0sv mike0sv mentioned this pull request Oct 5, 2022
Merged
@mike0sv mike0sv temporarily deployed to internal October 5, 2022 10:56 Inactive
@mike0sv mike0sv temporarily deployed to internal October 5, 2022 11:13 Inactive
@mike0sv mike0sv temporarily deployed to internal October 5, 2022 12:00 Inactive
@mike0sv mike0sv merged commit 8fed003 into release/0.3.0 Oct 5, 2022
@mike0sv mike0sv deleted the feature/deployments-cli branch October 5, 2022 12:23
@aguschin aguschin mentioned this pull request Oct 6, 2022
Merged
18 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@aguschin aguschin aguschin approved these changes

@omesser omesser Awaiting requested review from omesser

@madhur-tandon madhur-tandon Awaiting requested review from madhur-tandon

Assignees

@mike0sv mike0sv

Labels
None yet
Projects
No open projects
MLEM + GTO
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mike0sv @madhur-tandon @aguschin